src: use the config binding to carry --no-browser-globals #26228

joyeecheung · 2019-02-21T03:56:13Z

Instead of setting it in the process object, since this is
a configure-time option. Also added a shim that can be
deprecated and removed some time later.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2019-02-21T03:56:14Z

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2680/pipeline

danbev · 2019-02-21T04:23:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/20927/

BridgeAR · 2019-02-21T11:33:47Z

lib/internal/bootstrap/pre_execution.js

+  // it from the process. We may consider exposing it properly in
+  // process.features.
+  const { noBrowserGlobals } = internalBinding('config');
+  process._noBrowserGlobals = noBrowserGlobals;


This changes the behavior to the former.

Before, this variable was only set in case it was true. Now it's set unconditionally.
It is now also set as a regular property and not only as read-only.

BridgeAR · 2019-03-05T17:03:11Z

Ping @joyeecheung

Instead of setting it in the process object, since this is a configure-time option. Also added a shim that can be deprecated and removed some time later.

joyeecheung · 2019-03-06T10:59:16Z

@BridgeAR Thanks for review, updated.

CI: https://ci.nodejs.org/job/node-test-pull-request/21255/ ✔️

Instead of setting it in the process object, since this is a configure-time option. Also added a shim that can be deprecated and removed some time later. PR-URL: nodejs#26228 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR · 2019-03-06T23:29:15Z

Landed in 03c71a9 🎉

Instead of setting it in the process object, since this is a configure-time option. Also added a shim that can be deprecated and removed some time later. PR-URL: nodejs#26228 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Instead of setting it in the process object, since this is a configure-time option. Also added a shim that can be deprecated and removed some time later. PR-URL: #26228 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

zcbenz · 2019-03-29T07:14:02Z

This is actually causing some problems in Electron.

We load Node in 2 different environments:

Independent mode like the upstream one, which installs all browser globals
Browser mode which runs together with DOM and does not install browser globals.

By turning this config into compile-time only, we are not able to switch modes at runtime.

/cc @nitsakh @codebytere

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

joyeecheung · 2019-03-29T14:03:00Z

@zcbenz Hi,

I am pretty sure process.noBrowserGlobals was already processed pretty early during to bootstrap so after that it was not possible to change the browser globals at run time anyway? --no-browser-globals was already a build time flag before this patch, it was not a valid CLI option for node. This patch just moves process.noBrowserGlobals to process.binding('config').noBrowserGlobals and shim it later so that it would be easier to just remove process.noBrowserGlobals some time.

I think we can make the installation dynamic at run time, though process.noBrowserGlobals is not an ideal place to do that. For example, we can make process.features.browserGlobals an accessor property and toggle the globals dynamically

zcbenz · 2019-03-30T01:43:15Z

@joyeecheung We hijacked the bootstrap script to install some Electron specific things to Node. Having a dynamic flag would be great.

joyeecheung · 2019-03-30T03:41:23Z

@zcbenz If you just hijack the property passed into bootstrap script and only need it once, maybe the easiest workaround is to hijack process.binding('config') (in src/node_config.cc) instead.

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 21, 2019

danbev approved these changes Feb 21, 2019

View reviewed changes

jasnell approved these changes Feb 21, 2019

View reviewed changes

BridgeAR reviewed Feb 21, 2019

View reviewed changes

src: use the config binding to carry --no-browser-globals

b41afa4

Instead of setting it in the process object, since this is a configure-time option. Also added a shim that can be deprecated and removed some time later.

joyeecheung force-pushed the no-browser-globals branch from 59f5589 to b41afa4 Compare March 6, 2019 10:54

BridgeAR approved these changes Mar 6, 2019

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 6, 2019

BridgeAR closed this Mar 6, 2019

BridgeAR mentioned this pull request Mar 13, 2019

2019-03-14, Version 11.12.0 (Current) #26644

Merged

zcbenz added a commit to electron/node that referenced this pull request Mar 29, 2019

fix: make it possible to change noBrowserGlobals at runtime

21f70b0

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

zcbenz added a commit to electron/node that referenced this pull request Apr 10, 2019

fix: make it possible to change noBrowserGlobals at runtime

105f8b6

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

zcbenz added a commit to electron/node that referenced this pull request Apr 10, 2019

fix: make it possible to change noBrowserGlobals at runtime

91d764b

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

zcbenz added a commit to electron/node that referenced this pull request Apr 10, 2019

fix: make it possible to change noBrowserGlobals at runtime

e0ea7c2

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

zcbenz added a commit to electron/node that referenced this pull request Apr 10, 2019

fix: make it possible to change noBrowserGlobals at runtime

7a9f410

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

zcbenz added a commit to electron/node that referenced this pull request Apr 10, 2019

fix: make it possible to change noBrowserGlobals at runtime

45078b1

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

zcbenz added a commit to electron/node that referenced this pull request Apr 11, 2019

fix: make it possible to change noBrowserGlobals at runtime

9c75d38

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

nitsakh pushed a commit to electron/node that referenced this pull request Apr 11, 2019

fix: make it possible to change noBrowserGlobals at runtime

4c95007

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

nitsakh pushed a commit to electron/node that referenced this pull request Apr 11, 2019

fix: make it possible to change noBrowserGlobals at runtime

cb9029c

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

nitsakh pushed a commit to electron/node that referenced this pull request Apr 11, 2019

fix: make it possible to change noBrowserGlobals at runtime

5abb66e

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

nitsakh pushed a commit to electron/node that referenced this pull request Apr 11, 2019

fix: make it possible to change noBrowserGlobals at runtime

6854570

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

zcbenz added a commit to electron/node that referenced this pull request Apr 16, 2019

fix: make it possible to change noBrowserGlobals at runtime

3000279

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

zcbenz added a commit to electron/node that referenced this pull request Apr 16, 2019

fix: make it possible to change noBrowserGlobals at runtime

dc8cf8e

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

codebytere pushed a commit to electron/node that referenced this pull request Jun 20, 2019

fix: make it possible to change noBrowserGlobals at runtime

ebb8800

This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: use the config binding to carry --no-browser-globals #26228

src: use the config binding to carry --no-browser-globals #26228

joyeecheung commented Feb 21, 2019

nodejs-github-bot commented Feb 21, 2019

danbev commented Feb 21, 2019

BridgeAR Feb 21, 2019

BridgeAR commented Mar 5, 2019

joyeecheung commented Mar 6, 2019 •

edited by BridgeAR

Loading

BridgeAR commented Mar 6, 2019

zcbenz commented Mar 29, 2019

joyeecheung commented Mar 29, 2019

zcbenz commented Mar 30, 2019

joyeecheung commented Mar 30, 2019

src: use the config binding to carry --no-browser-globals #26228

src: use the config binding to carry --no-browser-globals #26228

Conversation

joyeecheung commented Feb 21, 2019

Checklist

nodejs-github-bot commented Feb 21, 2019

danbev commented Feb 21, 2019

BridgeAR Feb 21, 2019

Choose a reason for hiding this comment

BridgeAR commented Mar 5, 2019

joyeecheung commented Mar 6, 2019 • edited by BridgeAR Loading

BridgeAR commented Mar 6, 2019

zcbenz commented Mar 29, 2019

joyeecheung commented Mar 29, 2019

zcbenz commented Mar 30, 2019

joyeecheung commented Mar 30, 2019

joyeecheung commented Mar 6, 2019 •

edited by BridgeAR

Loading